Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#9] Placeholder service provider #30

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

casptyche
Copy link

Hi,

#9 - Apologies I didn't see another PR had been raised for this already & don't intend to create a 'race' of any sorts - I'm not looking for the IssueHunt.

I've implemented a very rough approach and just implemented an abstraction layer directly for PlaceholderApi and maintained PlaceholderApi's approach of leaving the translation of 'params' completely up to the external implementation, rather than looking to create a mapping within Tresor, though I appreciate this may not be what was in mind?

The end-goal in my head is that when adding other Providers in the future, a translation can be created between PAPI's format and their format.
Though I'm not aware of other providers / Placeholder plugins, I had a brief look at MVDW but it appeared that it's exclusively for MVDW's premium plugins and are all available via PAPI regardless?

  • Currently untested code

Let me know, I'm quite keen to contribute to some open source repos and this seemed like a good issue to pick up but if I've missed the intended mark I'm happy to adjust this or look at another issue (given the other PR raised for #9 too).

@casptyche casptyche marked this pull request as draft June 7, 2024 19:07
@casptyche
Copy link
Author

Marking as draft as code is untested

@Phoenix616
Copy link
Member

Thank you for your PR. Even though it's a duplicate it's quite interesting to see a different approach. You seem to be missing a way for another plugin to resolve/replace placeholders though? (You don't need to add that if you want to do something else now that you noticed there's a PR for it already. Depends on if @eyvallahabi wants to continue their PR I guess)

But I like the way placeholders can be registered by other plugins in comparison to the other PR, I just feel that it might be more complex than necessary as it basically just mirrors what PAPI does. (I don't really see the usefulness of the author, version and plugin being exposed in the API. Imo. that's an implementation detail of the service provider so a simple Placeholders#registerPlaceholder(Plugin, String id, BiFunction<OfflinePlayer, String, String>) might even be able to handle it with the PAPI extension being constructed inside the provider implementation)

@eyvallahabi maybe you can find a similar but simpler approach for your PR?

@casptyche
Copy link
Author

casptyche commented Jun 7, 2024

Thank you for your PR. Even though it's a duplicate it's quite interesting to see a different approach. You seem to be missing a way for another plugin to resolve/replace placeholders though? (You don't need to add that if you want to do something else now that you noticed there's a PR for it already. Depends on if @eyvallahabi wants to continue their PR I guess)

But I like the way placeholders can be registered by other plugins in comparison to the other PR, I just feel that it might be more complex than necessary as it basically just mirrors what PAPI does. (I don't really see the usefulness of the author, version and plugin being exposed in the API. Imo. that's an implementation detail of the service provider so a simple Placeholders#registerPlaceholder(Plugin, String id, BiFunction<OfflinePlayer, String, String>) might even be able to handle it with the PAPI extension being constructed inside the provider implementation)

@eyvallahabi maybe you can find a similar but simpler approach for your PR?

Thanks for taking the time to reply and look over. You're totally right, I forgot to implement an equivalent of PAPI's setPlaceholders().

My thinking behind copying over the author/version/plugin was that then a server owner or dev or whatever could then trace a placeholder through their provider (PAPI/whomever) to the original, rather than it being traced back to Tresor.
But I do agree it can be removed for the sake of simplifying this PR, and I did feel like I was being too verbose essentially copying over PAPI's interface to this MR :D.

The callback approach you mention probably be the most plugin agnostic approach, would be happy to adopt that too. If the other PR user doesn't come back in the next week or so I'll look at picking this back up, don't want to hijack something they're half-way through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants